Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Final token updates #12754

Merged

Conversation

marcellamaki
Copy link
Member

Summary

Resolves 3 outstanding categories of issues from the KDS upgrade:

  • ensures the second package.json is up to date
  • reverts a few changes that were the wrong color, as they had inadvertently been updated twice (from 1100 -> 600, and then 600 -> 300)
  • Updates tokens in a handful of missed files

References

Please see the first PR and the follow up comments for more details #12742

Reviewer guidance

Manual checks to confirm all is well:

  • does everything build and run properly
  • check the KDateRange component. Are there any regressions? (This is what tipped me off to the second package.json needing updating.)
  • Check the "end quiz" button - is it the correct shade of red?
  • otherwise - any glaring regressions?

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@github-actions github-actions bot added APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend labels Oct 25, 2024
// palette.red.v_300 but at the same time,
// palette.red.v_300 is the darkest available red
// palette.red.v_600 but at the same time,
// palette.red.v_600 is the darkest available red
// in the palette. Using this hardcoded color was
// agreed with designers.
':hover': { 'background-color': '#A81700' },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do a follow up issue to look for hard coded colours like this, and instead use the darken utilities in develop?

@@ -115,7 +115,7 @@
},
linkStyle() {
const hoverBg = this.isFullscreen
? this.$themeBrand.secondary.v_300
? this.$themeBrand.secondary.v_600
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow up from: #12742 (comment)

@@ -63,7 +63,7 @@
return {
color: this.$themeTokens.text,
':hover': {
'background-color': this.$themeBrand.secondary.v_300,
'background-color': this.$themeBrand.secondary.v_600,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow up from: #12742 (comment)

@@ -69,7 +69,7 @@
iconStyle() {
if (this.hasSuperAdminPermission) {
return {
fill: this.lightIcon ? this.$themePalette.yellow.v_400 : this.$themeTokens.superAdmin,
fill: this.lightIcon ? this.$themePalette.yellow.v_200 : this.$themeTokens.superAdmin,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct mapping.

@@ -9,7 +9,7 @@
</div>
<div
class="progress-bar-wrapper"
:style="{ backgroundColor: $themePalette.grey.v_200 }"
:style="{ backgroundColor: $themePalette.grey.v_300 }"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct mapping.

@@ -42,7 +42,7 @@
return this.$computedClass({
':focus': this.$coreOutline,
':hover': {
backgroundColor: this.$themePalette.blue.v_200,
backgroundColor: this.$themePalette.blue.v_100,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct mapping.

@@ -341,10 +341,10 @@
cancelStyleOverrides() {
return {
color: this.$themeTokens.textInverted,
'background-color': this.$themePalette.red.v_300,
'background-color': this.$themePalette.red.v_600,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow up from: #12742 (comment)

@@ -3,7 +3,7 @@
<div
v-show="userDevicesUsingIE11"
class="alert"
:style="{ backgroundColor: $themePalette.yellow.v_200 }"
:style="{ backgroundColor: $themePalette.yellow.v_100 }"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow up from here: #12742 (comment)

@@ -98,7 +98,7 @@
return this.$computedClass({
':focus': this.$coreOutline,
':hover': {
backgroundColor: this.$themePalette.blue.v_200,
backgroundColor: this.$themePalette.blue.v_100,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct mapping.

@marcellamaki marcellamaki merged commit ecbd748 into learningequality:release-v0.17.x Oct 25, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants